feat: improve application update logic and validation#2561
Merged
iamitprakash merged 9 commits intodevelopfrom Feb 10, 2026
Merged
feat: improve application update logic and validation#2561iamitprakash merged 9 commits intodevelopfrom
iamitprakash merged 9 commits intodevelopfrom
Conversation
…ror handling - Added a new error message for editing applications too soon. - Implemented a function to build the update payload for applications. - Updated the application update logic to include user authorization and time-based restrictions. - Refactored the application validator to include comprehensive validation for update data. - Adjusted routes to use the new validation function for application updates.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR implements a 24-hour edit cooldown mechanism for application updates. It introduces an error constant, refactors the update controller with a payload builder, expands validation to distinguish between update and feedback scenarios, adds authorization and cooldown checks in the model layer, and introduces a new PATCH route for applications while updating tests to match the revised signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Validator
participant Model
participant Firestore
Client->>Controller: PATCH /applications/:id<br/>(update data, auth)
Controller->>Validator: validateApplicationUpdateData()
Validator-->>Controller: ✓ valid | ✗ 400
Controller->>Controller: buildApplicationUpdatePayload()
Controller->>Model: updateApplication(payload, id, userId)
rect rgba(100, 150, 200, 0.5)
Note over Model: Transaction Start
Model->>Firestore: Get application by id
alt Application not found
Model-->>Controller: {status: notFound}
else userId mismatch
Model-->>Controller: {status: unauthorized}
else lastEditAt < 24 hours ago
Model-->>Controller: {status: tooSoon}
else Valid
Model->>Firestore: Update data + lastEditAt
Model-->>Controller: {status: success}
end
Note over Model: Transaction End
end
Controller->>Firestore: Log update (on success only)
Controller-->>Client: 200 success | 404/401/409 error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ); | ||
| router.get("/:applicationId", authenticate, authorizeRoles([SUPERUSER]), applications.getApplicationById); | ||
| router.post("/", authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||
| router.patch("/:applicationId", authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 25 days ago
In general, the problem is fixed by adding a rate-limiting middleware to routes that perform authenticated, potentially expensive operations (like updating an application). In an Express app, this is typically done by using a well-known library such as express-rate-limit and applying a limiter either to all routes in the router or specifically to sensitive routes (e.g., write operations).
For this file, the least intrusive and most effective fix is to:
- Import
express-rate-limit. - Define a limiter instance tailored for these routes (e.g., a reasonable per-IP cap over a time window).
- Insert that limiter as middleware in the relevant routes’ middleware chains.
To cover the flagged route and the other similar, authenticated write routes, we can define a single limiter and apply it to all modifying endpoints (POST /, PATCH /:applicationId, PATCH /:applicationId/feedback, and PATCH /:applicationId/nudge). This preserves existing authentication/authorization/validation and only adds an extra guard against abuse. Concretely in routes/applications.ts, we will:
- Add
const rateLimit = require("express-rate-limit");near the top. - Define a limiter, e.g.
const applicationWriteLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 100 });after the router is created. - Update the relevant route definitions to include
applicationWriteLimiterbeforeauthenticate(or immediately after, either is acceptable as long as it runs early) in the middleware arrays.
Suggested changeset
2
routes/applications.ts
| @@ -5,9 +5,15 @@ | ||
| const applications = require("../controllers/applications"); | ||
| const { authorizeOwnOrSuperUser } = require("../middlewares/authorizeOwnOrSuperUser"); | ||
| const applicationValidator = require("../middlewares/validators/application"); | ||
| const rateLimit = require("express-rate-limit"); | ||
|
|
||
| const router = express.Router(); | ||
|
|
||
| const applicationWriteLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 write requests per windowMs | ||
| }); | ||
|
|
||
| router.get( | ||
| "/", | ||
| authenticate, | ||
| @@ -16,15 +19,16 @@ | ||
| applications.getAllOrUserApplication | ||
| ); | ||
| router.get("/:applicationId", authenticate, authorizeRoles([SUPERUSER]), applications.getApplicationById); | ||
| router.post("/", authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||
| router.patch("/:applicationId", authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); | ||
| router.post("/", applicationWriteLimiter, authenticate, applicationValidator.validateApplicationData, applications.addApplication); | ||
| router.patch("/:applicationId", applicationWriteLimiter, authenticate, applicationValidator.validateApplicationUpdateData, applications.updateApplication); | ||
| router.patch( | ||
| "/:applicationId/feedback", | ||
| applicationWriteLimiter, | ||
| authenticate, | ||
| authorizeRoles([SUPERUSER]), | ||
| applicationValidator.validateApplicationFeedbackData, | ||
| applications.submitApplicationFeedback | ||
| ); | ||
| router.patch("/:applicationId/nudge", authenticate, applications.nudgeApplication); | ||
| router.patch("/:applicationId/nudge", applicationWriteLimiter, authenticate, applications.nudgeApplication); | ||
|
|
||
| module.exports = router; |
package.json
Outside changed files
| @@ -42,7 +42,8 @@ | ||
| "passport-github2": "0.1.12", | ||
| "passport-google-oauth20": "^2.0.0", | ||
| "rate-limiter-flexible": "5.0.3", | ||
| "winston": "3.13.0" | ||
| "winston": "3.13.0", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/chai": "4.3.16", |
This fix introduces these dependencies
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
Copilot is powered by AI and may make mistakes. Always verify output.
AnujChhikara
commented
Jan 29, 2026
| }); | ||
| }); | ||
|
|
||
| describe("updateApplication", function () { |
Contributor
Author
There was a problem hiding this comment.
will refactor and add this test in the test pr of the edit applications
AnujChhikara
commented
Jan 29, 2026
| feedback: "some feedback", | ||
| }; | ||
| await applicationValidator.validateApplicationUpdateData(req, res, nextSpy); | ||
| await applicationValidator.validateApplicationFeedbackData(req, res, nextSpy); |
Contributor
Author
There was a problem hiding this comment.
need to update these as changes the validator name
AnujChhikara
commented
Jan 29, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middlewares/validators/application.ts (1)
107-112: Fix misleading log message.The error log says “recruiter data” but this validator handles application feedback. This will confuse debugging and alerting.
🔧 Suggested fix
- logger.error(`Error in validating recruiter data: ${error}`); + logger.error(`Error in validating application feedback data: ${error}`);
🤖 Fix all issues with AI agents
In `@controllers/applications.ts`:
- Around line 125-133: The updateApplication handler currently calls
ApplicationModel.updateApplication even when
buildApplicationUpdatePayload(rawBody) returns an empty object, causing only
lastEditAt to change and producing misleading success logs; modify the
updateApplication function to check the returned dataToUpdate (from
buildApplicationUpdatePayload) and if it has no own enumerable properties (i.e.,
no updatable fields), respond with a 400/422 error and short-circuit without
calling ApplicationModel.updateApplication or logging success—otherwise proceed
as before using userId and applicationId.
In `@middlewares/validators/application.ts`:
- Around line 141-163: The update schema currently allows an empty object
because no keys are required; modify the schema (the joi.object() assigned to
schema in middlewares/validators/application.ts) to require at least one field
by adding a constraint such as .min(1) (or alternatively
.or('imageUrl','foundFrom','introduction','forFun','funFact','whyRds','numberOfHours','professional','socialLink'))
to the object chain so that empty payloads are rejected while keeping existing
validators like customWordCountValidator, professionalSchema and
socialLinkSchema intact.
In `@routes/applications.ts`:
- Around line 20-26: The two PATCH routes (router.patch("/:applicationId", ...
applications.updateApplication) and router.patch("/:applicationId/feedback", ...
applications.submitApplicationFeedback)) lack rate limiting; import and apply
the project's standard write-rate-limit middleware (symbol name
writeRateLimiter) to both routes and place it after authenticate and before the
validators/handlers so the order becomes: authenticate, writeRateLimiter,
applicationValidator.*, then the controller (applications.updateApplication /
applications.submitApplicationFeedback).
In `@test/integration/application.test.ts`:
- Around line 691-693: The test is mutating lastNudgeAt via
applicationModel.updateApplication which now enforces edit cooldown and touches
lastEditAt, coupling the test to edit logic; change the setup to directly update
the document (bypassing updateApplication) or use a dedicated test helper to
backdate lastNudgeAt so you only modify lastNudgeAt without triggering cooldown
logic—locate the call using applicationModel.updateApplication,
nudgeApplicationId and userId and replace it with a direct DB/document update or
helper that sets lastNudgeAt to twentyFiveHoursAgo while leaving lastEditAt
untouched.
AnujChhikara
commented
Jan 29, 2026
…ow a maximum of 168 hours
MayankBansal12
previously approved these changes
Feb 5, 2026
Suvidh-kaushik
requested changes
Feb 6, 2026
iamitprakash
requested changes
Feb 6, 2026
iamitprakash
approved these changes
Feb 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Date: 30 Jan 2026
Developer Name: @AnujChhikara
Issue Ticket Number
Tech Doc
Onboarding Link
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes